-
Notifications
You must be signed in to change notification settings - Fork 10
Base Configs Support - Phase 1 #455
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Currently, the `ValidateConfig` and the
`createImageCustomizerParameters` functions share some
responsibilities. The `createImageCustomizerParameters` function is
responsible for merging the CLI values with the config values and for
setting up dynamic values (e.g. image UUID). It also validates some
CLI args (e.g. rpm sources). The `ValidateConfig` is responsible for
validating the config. But it also sometimes validates CLI args (e.g.
input image path). It also needs to not validate unused values (e.g.
config's input image path if the CLI option is specified). This mixed
responsibilities has created a somewhat tight coupling between the two
functions.
This change merges the `createImageCustomizerParameters` function into
`ValidateConfig` so that `ValidateConfig` is responsible for all
validation logic and all config/options merging logic. This will
hopefully make the code easier to maintain by ensuring all related
logic is in one location.
Changes include:
1. Merge `createImageCustomizerParameters` into `ValidateConfig`.
2. Create a new class called `ResolvedConfig` to store all merged
config/options values.
3. Keep `imageCustomizerParameters` for passing around image metadata.
4. Update `ImageCustomizerOptions`:
1. Add an `IsValid` function to centralize validity checking of its
fields.
2. Change type of `OutputImageFormat` and `PackageSnapshotTime` to
use API lib's types.
5. Remove `ImageCreatorParameters` since `ResolvedConfig` can be used
instead.
6. Handle merging package snapshot CLI vs. config in `ValidateConfig`
instead of in `addRemoveAndUpdatePackages`.
Split the `ImageCustomizerParameters` into two, with a new `ResolvedConfig` struct taking all the merged config values and any generated values, and leaving `ImageCustomizerParameters` to handle passing around image metadata. As part of this, some redundant values in `ResolvedConfig` are being changed from fields to functions. This change is being done in preparation for the change that will merge the config merging logic into the `ValidateConfig` function. Also, handle the merging of the packages snapshot time when generating the `ResolvedConfig` instead of deep within the code stack, to match how other CLI parameters are handled. Also, refactor some functions to take `ResolvedConfig` instead of a long list of parameters, to help simplify code. Also, this will make the upcoming Hierarchical Config changes a little eaiser.
functions share some responsibilities. The `createResolvedConfig` function is responsible for merging the CLI values with the config values and for setting up dynamic values (e.g. image UUID). It also validates some CLI args (e.g. rpm sources). The `ValidateConfig` is responsible for validating the config. But it also sometimes validates CLI args (e.g. input image path). It also needs to not validate unused values (e.g. config's input image path if the CLI option is specified). This mixed responsibilities has created a somewhat tight coupling between the two functions. This change merges the `createResolvedConfig` function into `ValidateConfig` so that `ValidateConfig` is responsible for all validation logic and all config/options merging logic. This will hopefully make the code easier to maintain by ensuring all related logic is in one location. Also, in the Image Creator code, replace `ImageCreatorParameters` with `ResolvedConfig`.
|
|
||
| Added in v1.1.0. | ||
|
|
||
| ## output [[output](./output.md)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to add the baseConfigs field to this file.
| InputImageFile string | ||
|
|
||
| // Output artifacts | ||
| OutputArtifacts *imagecustomizerapi.Artifacts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use this value in the CustomizeImageOptions function?
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestBaseConfigsInputAndOutput(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to have at least one test that actually runs through an actual image customization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to add a full run functional test, any suggestions on how I can obtain a test image from which I can extract 2 artifacts (to test merged artifact items) ? I think current empty.vhdx would not work. if it is not ok to check in a non empty vhdx, can I exclude the artifacts item in the full run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the checkSkipForCustomizeDefaultImage function. It is used quite extensively in the functional tests.
You can pass in the base image to the test using the --base-image-core-efi-azl3 CLI option when running go test.
…mage-tools into user/elaine/poc1
| return | ||
| } | ||
|
|
||
| assert.FileExists(t, outImageFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to actually verify what's in the image.
For example, see TestCustomizeImageHostname for how to verify the hostname.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks. Will update in next pr
This PR introduces hierarchical configuration support in Image Customizer, allowing users to compose configs on top of one another via the .baseConfigs field.
Implement circular dependency detection
Create config inheritance chain resolution
include fields below:
Checklist